Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix zoom centering #605

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from
Draft

Fix zoom centering #605

wants to merge 8 commits into from

Conversation

guyguy2001
Copy link
Contributor

No description provided.

} else {
this._endOffsetX = this._width / 2;
this._endOffsetY = this._height / 2;
// TODO: should this be different? I need to respect if the mosue moved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this - I feel like something should break if the user moves their mouse while zooming, but I couldn't really find an issue when testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I finally found it:

chrome_sKjIFNZXIw.mp4

What happens is that if I zoom in, and while the animation is playing, move my mouse and then zoom in again, the mouse will not stay in the same world position (in this case it started outside the RNA, just to its right, but finished on the RNA)

I couldn't find a trivial and clean way to do this (which would happen by giving the screen/world conversion functions the current spacing in the middle of the animation, which isn't part of the ZOOM_SPACING array, but rather a linear value between the two).
I could calculate the value there, but that would mean that we'd calculate the progression in 3 different places, which I think is overkill.

Should I work on fixing that? Or should I document it as a known issue?

Copy link
Contributor Author

@guyguy2001 guyguy2001 Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not sure if that would be enough to fix it, since the animation has to finish before we can zoom in again (or before the next zoom-in applies, IDK), so the mouse would have to move in world space either way

EDIT: No, that's probably wrong. I think the new animation starts immidiately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open a new issue and resolve this thread before merging

this._endOffsetX = this._width / 2;
this._endOffsetY = this._height / 2;
// TODO: should this be different? I need to respect if the mosue moved
// also what if the user presses the button?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to add a parameter to allow it to zoom into the center of the RNA, for when the user presses the button.
This would also be useful if we'd want to allow users to choose in the settings whether they want the zoom to zoom to the mouse or to the center of the RNA.

Copy link
Contributor Author

@guyguy2001 guyguy2001 May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A problem with that is that the +/- hotkeys just simulate clicking the button, and so it isn't possible to (easily) make them behave differently.
I think that it's very out of scope for now, and it only really affects users who do use the mouse but don't use the scroll wheel
EDIT: This means that I want to make the +/- keys scroll to the center of the screen

this._endOffsetY = this._height / 2;
// TODO: should this be different? I need to respect if the mosue moved
// also what if the user presses the button?
// also what if the user drags in the middle of the zoom?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dragging in the middle of the zoom is broken in live, and I don't see myself fixing it.

@guyguy2001
Copy link
Contributor Author

Looks like I haven't documented this anywhere, but I need to support touch with these changes

@luxaritas
Copy link
Member

@guyguy2001 Out of curiosity, what is the status of this PR? Should we convert to a draft?

@guyguy2001
Copy link
Contributor Author

Uh I'm not sure
I have some uncommited code, which I need to clean up and push here; I basically tried to make it way too generic and it had a bit of scope creep.
I will probably sit on this one weekend when I have the motivation to, but I'm not sure when this will happen.

Overall all that's left it to either reduce the scope or complete the changes I started making (which shouldn't take a lot of time), and figure out what to do wit touch zooming.
If you want me to give you all of the changes and to finish this yourself, I can push everything next week. If not, I'll get to it somewhen

(Idk whether or not it should count as a draft given all this, feel free to choose)

@luxaritas
Copy link
Member

Going to mark this as a draft since there's still more work to be done. Feel free to mark it as ready later

@luxaritas luxaritas marked this pull request as draft November 18, 2022 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants